-
-
Notifications
You must be signed in to change notification settings - Fork 10
Add palettes and effects #110
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
frontend ======= - Field renderer: add genPalPrev, add type palette (palette preview) backend ======= - Effects: add ColorTwinkle, Plasma, Julia, Police - WIP - Lights control: add WLED palettes and show previews
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Palette data src/MoonLight/Modules/palettes.h |
New file: 60+ gradient palettes as byte arrays plus gGradientPalettes and palette_names. Large static data blob to review. |
Lights control / UI defs src/MoonLight/Modules/ModuleLightsControl.h |
Changes palette control to palette type, populates built-in + palettes.h entries, generates hex color payloads per palette, and loads dynamic gradient palettes at runtime. Review buffer construction and dynamic load path. |
Frontend palette preview & link rendering interface/src/lib/components/moonbase/FieldRenderer.svelte |
Adds genPalPrev() to build CSS linear-gradient preview from hex palettes; adds UI branch for palette control with preview; simplifies IP/mDNS anchor rendering. |
Drivers: audio & infrared src/MoonLight/Nodes/Drivers/D_AudioSync.h, src/MoonLight/Nodes/Drivers/D_Infrared.h |
AudioSync: adds getCRGBForBand and getAudioPalette and runtime loading of audio-driven palettes. Infrared: increases max palette index to cover expanded palette set. Check memory for runtime palettes. |
Effects: registration & new effects src/MoonLight/Modules/ModuleEffects.h, src/MoonLight/Nodes/Effects/E_WLED.h |
Reorders StarSkyEffect registration/allocation; adds four effects (ColorTwinkleEffect, PlasmaEffect, JuliaEffect, PoliceEffect) with controls and lifecycle methods. Note duplicated declarations of these classes in E_WLED.h. |
Layer mapping correction src/MoonLight/Layers/VirtualLayer.h, src/MoonLight/Layers/VirtualLayer.cpp |
Adds VirtualLayer::presetCorrection(nrOfLights_t&) and replaces inline preset index tweaks with this centralized call (applies for RGB2040). |
Module concurrency & updates src/MoonBase/Module.h, src/MoonBase/Module.cpp |
ModuleState::update early-returns on empty data; postUpdate/getUpdate use short timeouts/retry instead of blocking indefinitely. |
Docs docs/moonbase/devices.md |
Minor wording edits: removed “(controls module)” and changed phrasing to access device “via its IP address”. |
Sequence Diagram
sequenceDiagram
actor User
participant UI as FieldRenderer.svelte
participant Control as ModuleLightsControl
participant Pal as palettes.h
participant Driver as AudioSyncDriver
participant Effect as WLED_Effect
User->>UI: select palette
UI->>UI: genPalPrev(hex) → render preview
UI->>Control: send palette selection (name/index)
Control->>Pal: lookup palette bytes by index
Pal-->>Control: return gradient byte data
Control->>Control: build hex/temp buffer
Control->>Driver: loadDynamicGradientPalette(buffer) on layer
Driver->>Effect: apply palette to layer
Effect->>Effect: render loop uses new palette
Effect-->>User: LEDs updated
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
- starsky effect #96 — Related: StarSkyEffect implementation/registration overlaps with ModuleEffects adjustments.
- prep71 #106 — Related: ModuleState/module concurrency and update/mutex handling similar to Module.h/.cpp edits.
- v0.7.0 preparations #76 — Related: FieldRenderer palette/IP rendering and frontend palette UI adjustments.
Poem
🐰
I nibble gradients, byte by byte,
Sixty hues that twinkle bright,
Plasma swirls and Julia spins,
I hop and load the color bins,
LEDs bloom — a rabbit's delight!
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 34.69% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title 'Add palettes and effects' accurately summarizes the main changes in the PR, which introduce new color palettes and effect classes throughout the codebase. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing touches
- 📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands and usage tips.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In `@docs/moonbase/devices.md`:
- Around line 9-10: The possessive "it's" is incorrect in the device
instructions; update the text "Click on IP to go to the device via it's IP
address" to use the possessive form "its" instead. Locate the sentence in
docs/moonbase/devices.md (the line containing "Click on IP to go to the device
via it's IP address") and replace "it's" with "its" so it reads "Click on IP to
go to the device via its IP address".
In `@src/MoonLight/Modules/ModuleLightsControl.h`:
- Around line 281-355: The loop that builds hexString for gGradientPalettes uses
a hard cap while (j < 100) which can truncate palettes—replace this fixed limit
with a safe bounds check tied to the actual palette byte length (or a defined
MAX_PALETTE_BYTES) and iterate in 4-byte steps until you encounter an index==255
termination; specifically update the loop that reads from gGradientPalettes[i]
(the variables palette and j) so it ensures (j + 4) <= palette_length (or <=
MAX_PALETTE_BYTES) before reading the next 4 bytes, append the 4 bytes per
entry, and break when palette[j] (the entry index) == 255 to avoid truncation.
- Around line 425-427: The fixed-size memcpy truncates palettes and drops the
0xFF terminator; change to copying from PROGMEM byte-by-byte using
pgm_read_byte() from gGradientPalettes[index] (index computed as
updatedItem.value.as<uint8_t>() - 11) into tcp until you encounter 0xFF or reach
the tcp buffer limit, then ensure tcp contains a valid 0xFF terminator before
calling layerP.palette.loadDynamicGradientPalette(tcp); also ensure tcp’s size
is sufficient for the largest expected palette and set the final byte to 0xFF if
the loop exits due to the size limit.
In `@src/MoonLight/Modules/palettes.h`:
- Around line 28-31: The header uses the Arduino type `byte` for `ib_jul01_gp`
but doesn't include Arduino.h, making compilation order-dependent; update the
header to be self-contained by adding a guarded include or fallback typedef:
check for a prior definition of `byte` (or for ARDUINO macro) and if missing
either include <Arduino.h> or typedef unsigned char byte so `const byte
ib_jul01_gp[]` has a defined type within this header.
In `@src/MoonLight/Nodes/Drivers/D_Infrared.h`:
- Around line 367-369: The clamp for palette increase in the combined_code ==
codePaletteInc branch currently uses MIN(state.data["palette"].as<uint8_t>() +
1, 8 + 3 + 61) which allows an out‑of‑range index (72); change the ceiling to
the last valid index by subtracting 1 (i.e. use (8 + 3 + 61) - 1 or, better, a
shared MAX_PALETTE_INDEX constant) so newState["palette"] is clamped to
0..(count-1); update the same logic location (the combined_code ==
codePaletteInc branch that sets newState["palette"]) and ensure any other uses
of the literal 8+3+61 are replaced with the shared constant or consistently
subtract 1.
In `@src/MoonLight/Nodes/Effects/E_WLED.h`:
- Around line 2143-2254: The Julia and Police effect loop() implementations are
empty causing no output; fix by implementing a minimal placeholder render in
each effect's loop (e.g., in the loop() method in E_WLED.h for Julia and
similarly for Police around the region noted at 2304-2308): call
SEGMENT.setUpLeds(), clear the frame with SEGMENT.fill(BLACK) (or BLACK with
desired brightness), then draw a simple deterministic indicator (for example a
single moving pixel or a low-cost color stripe using SEGMENT.setPixelColorXY or
SEGMENT.setPixelColor) based on strip.now or SEGMENT.custom1 so the effect
visibly updates, and return; alternatively, if you prefer not to ship a
placeholder, unregister/remove these effects from the registry until full ports
are implemented. Ensure you reference and modify the loop() methods for the
Julia and Police effect classes and use SEGMENT APIs (setUpLeds, fill,
setPixelColorXY) to avoid leaving previous frames visible.
- Around line 2010-2063: The loop() for this effect (the overridden loop() in
E_WLED.h / ColorTwinkle) is currently empty and leaves stale LEDs; add a minimal
placeholder: on first call (check SEGENV.call == 0) call SEGMENT.setUpLeds()
then clear the strip by setting every pixel to BLACK (use SEGLEN and
SEGMENT.setPixelColor), or alternatively call SEGMENT.fill(BLACK) to ensure no
stale output, and ensure any required SEGENV.allocateData(...) is invoked if the
effect expects per-pixel state (use SEGENV.allocateData with dataSize computed
from SEGLEN) so the effect doesn’t crash; keep the implementation tiny and
deterministic until the full twinkle logic is ported.
- Around line 2068-2109: PlasmaEffect currently advertises 3D via dim() but only
writes a single column in loop() (layer->setRGB(Coord3D(0, i), ...)), leaving
other LEDs untouched; either change dim() to return _1D to match current
rendering, or modify loop() to iterate X and Z (and optionally clear the layer
first) and call layer->setRGB(Coord3D(x, i, z), ColorFromPalette(...)) for each
coordinate so the effect covers the full 2D/3D plane; update the dim()
implementation or the loop() iteration accordingly in the PlasmaEffect class.
🧹 Nitpick comments (2)
src/MoonLight/Modules/ModuleEffects.h (1)
138-141: Keep WLED effects list alphabetical.The section is labeled “alphabetically” but the new effects are out of order. Reordering keeps UI and allocation lists consistent.
♻️ Suggested reorder
- addControlValue(control, getNameAndTags<ColorTwinkleEffect>()); - addControlValue(control, getNameAndTags<PlasmaEffect>()); - addControlValue(control, getNameAndTags<JuliaEffect>()); - addControlValue(control, getNameAndTags<PoliceEffect>()); + addControlValue(control, getNameAndTags<ColorTwinkleEffect>()); + addControlValue(control, getNameAndTags<JuliaEffect>()); + addControlValue(control, getNameAndTags<PlasmaEffect>()); + addControlValue(control, getNameAndTags<PoliceEffect>());- if (!node) node = checkAndAlloc<ColorTwinkleEffect>(name); - if (!node) node = checkAndAlloc<PlasmaEffect>(name); - if (!node) node = checkAndAlloc<JuliaEffect>(name); - if (!node) node = checkAndAlloc<PoliceEffect>(name); + if (!node) node = checkAndAlloc<ColorTwinkleEffect>(name); + if (!node) node = checkAndAlloc<JuliaEffect>(name); + if (!node) node = checkAndAlloc<PlasmaEffect>(name); + if (!node) node = checkAndAlloc<PoliceEffect>(name);Also applies to: 235-238
interface/src/lib/components/moonbase/FieldRenderer.svelte (1)
189-207: Move the inline<style>block to component scope.Embedding
<style>inside the template inserts a<style>element per row and bypasses Svelte’s scoped styling. Prefer a top-level<style>section.♻️ Suggested change (remove inline style)
- <style> - .palette-preview { - width: 250px; - height: 40px; - border: 1px solid `#ccc`; - border-radius: 3px; - } - </style>Add this at the bottom of the component instead:
<style> .palette-preview { width: 250px; height: 40px; border: 1px solid `#ccc`; border-radius: 3px; } </style>
| * Click on the name to go to the device via mDNS | ||
| * Click on IP to go to the device via it's IP address |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix possessive “its”.
“it’s” → “its”.
✏️ Proposed fix
- * Click on IP to go to the device via it's IP address
+ * Click on IP to go to the device via its IP address📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * Click on the name to go to the device via mDNS | |
| * Click on IP to go to the device via it's IP address | |
| * Click on the name to go to the device via mDNS | |
| * Click on IP to go to the device via its IP address |
🤖 Prompt for AI Agents
In `@docs/moonbase/devices.md` around lines 9 - 10, The possessive "it's" is
incorrect in the device instructions; update the text "Click on IP to go to the
device via it's IP address" to use the possessive form "its" instead. Locate the
sentence in docs/moonbase/devices.md (the line containing "Click on IP to go to
the device via it's IP address") and replace "it's" with "its" so it reads
"Click on IP to go to the device via its IP address".
| control = addControl(controls, "palette", "palette"); // palette type | ||
| control["default"] = 9; | ||
| addControlValue(control, "Cloud"); | ||
| addControlValue(control, "Lava"); | ||
| addControlValue(control, "Ocean"); | ||
| addControlValue(control, "Forest"); | ||
| addControlValue(control, "Rainbow"); | ||
| addControlValue(control, "RainbowStripe"); | ||
| addControlValue(control, "Party"); | ||
| addControlValue(control, "Heat"); | ||
| addControlValue(control, "Random"); | ||
| addControlValue(control, "MoonModules"); | ||
| addControlValue(control, "Orange"); | ||
|
|
||
| control["values"].to<JsonArray>(); | ||
|
|
||
| const char* const builtInPaletteNames[] = {"Cloud", "Lava", "Ocean", "Forest", "Rainbow", "RainbowStripe", "Party", "Heat"}; | ||
| const CRGBPalette16 builtInPalettes[] = {CloudColors_p, LavaColors_p, OceanColors_p, ForestColors_p, RainbowColors_p, RainbowStripeColors_p, PartyColors_p, HeatColors_p}; | ||
| for (int i = 0; i < sizeof(builtInPaletteNames) / sizeof(char*); i++) { | ||
| JsonArray values = control["values"]; | ||
| JsonObject object = values.add<JsonObject>(); | ||
| object["name"] = builtInPaletteNames[i]; | ||
|
|
||
| // Convert CRGBPalette16 to hex string | ||
| String hexString = ""; | ||
| const CRGBPalette16 pal = builtInPalettes[i]; | ||
|
|
||
| char buf[3]; | ||
| for (int j = 0; j < 16; j++) { | ||
| // Add index (0, 16, 32, ... 240) | ||
| sprintf(buf, "%02x", j * 16); | ||
| hexString += buf; | ||
|
|
||
| // Add R, G, B | ||
| sprintf(buf, "%02x", pal[j].r); | ||
| hexString += buf; | ||
| sprintf(buf, "%02x", pal[j].g); | ||
| hexString += buf; | ||
| sprintf(buf, "%02x", pal[j].b); | ||
| hexString += buf; | ||
| } | ||
|
|
||
| // Add final entry at index 255 | ||
| sprintf(buf, "%02x", 255); | ||
| hexString += buf; | ||
| sprintf(buf, "%02x", pal[15].r); | ||
| hexString += buf; | ||
| sprintf(buf, "%02x", pal[15].g); | ||
| hexString += buf; | ||
| sprintf(buf, "%02x", pal[15].b); | ||
| hexString += buf; | ||
|
|
||
| object["colors"] = hexString; | ||
| } | ||
|
|
||
| const char* const customPaletteNames[] = {"Random", "MoonModules", "Orange"}; | ||
| for (int i = 0; i < sizeof(customPaletteNames) / sizeof(char*); i++) { | ||
| JsonArray values = control["values"]; | ||
| JsonObject object = values.add<JsonObject>(); | ||
| object["name"] = customPaletteNames[i]; | ||
| object["colors"] = ""; | ||
| } | ||
|
|
||
| // add palettes from palettes.h | ||
| for (int i = 0; i < sizeof(palette_names) / sizeof(char*); i++) { | ||
| JsonArray values = control["values"]; | ||
| JsonObject object = values.add<JsonObject>(); | ||
| object["name"] = palette_names[i]; | ||
|
|
||
| String hexString = ""; | ||
| const byte* palette = gGradientPalettes[i]; | ||
| int j = 0; | ||
|
|
||
| // Read 4-byte entries (index, r, g, b) until index == 255 | ||
| while (j < 100) { // Safety limit | ||
| for (int k = 0; k < 4; k++) { | ||
| char buf[3]; | ||
| sprintf(buf, "%02x", palette[j++]); | ||
| hexString += buf; | ||
| } | ||
| // Check if we just wrote the final entry (index was 255) | ||
| if (palette[j - 4] == 255) break; | ||
| } | ||
|
|
||
| object["colors"] = hexString; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Palette preview generation can truncate larger gradients.
The while (j < 100) cap will cut off palettes longer than 100 bytes (some in palettes.h exceed that), producing incomplete previews. Consider raising the cap and guarding by 4‑byte entries.
🛠️ Suggested tweak
- while (j < 100) { // Safety limit
+ const size_t kMaxPaletteBytes = 256; // allow larger gradients
+ while (j + 3 < kMaxPaletteBytes) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| control = addControl(controls, "palette", "palette"); // palette type | |
| control["default"] = 9; | |
| addControlValue(control, "Cloud"); | |
| addControlValue(control, "Lava"); | |
| addControlValue(control, "Ocean"); | |
| addControlValue(control, "Forest"); | |
| addControlValue(control, "Rainbow"); | |
| addControlValue(control, "RainbowStripe"); | |
| addControlValue(control, "Party"); | |
| addControlValue(control, "Heat"); | |
| addControlValue(control, "Random"); | |
| addControlValue(control, "MoonModules"); | |
| addControlValue(control, "Orange"); | |
| control["values"].to<JsonArray>(); | |
| const char* const builtInPaletteNames[] = {"Cloud", "Lava", "Ocean", "Forest", "Rainbow", "RainbowStripe", "Party", "Heat"}; | |
| const CRGBPalette16 builtInPalettes[] = {CloudColors_p, LavaColors_p, OceanColors_p, ForestColors_p, RainbowColors_p, RainbowStripeColors_p, PartyColors_p, HeatColors_p}; | |
| for (int i = 0; i < sizeof(builtInPaletteNames) / sizeof(char*); i++) { | |
| JsonArray values = control["values"]; | |
| JsonObject object = values.add<JsonObject>(); | |
| object["name"] = builtInPaletteNames[i]; | |
| // Convert CRGBPalette16 to hex string | |
| String hexString = ""; | |
| const CRGBPalette16 pal = builtInPalettes[i]; | |
| char buf[3]; | |
| for (int j = 0; j < 16; j++) { | |
| // Add index (0, 16, 32, ... 240) | |
| sprintf(buf, "%02x", j * 16); | |
| hexString += buf; | |
| // Add R, G, B | |
| sprintf(buf, "%02x", pal[j].r); | |
| hexString += buf; | |
| sprintf(buf, "%02x", pal[j].g); | |
| hexString += buf; | |
| sprintf(buf, "%02x", pal[j].b); | |
| hexString += buf; | |
| } | |
| // Add final entry at index 255 | |
| sprintf(buf, "%02x", 255); | |
| hexString += buf; | |
| sprintf(buf, "%02x", pal[15].r); | |
| hexString += buf; | |
| sprintf(buf, "%02x", pal[15].g); | |
| hexString += buf; | |
| sprintf(buf, "%02x", pal[15].b); | |
| hexString += buf; | |
| object["colors"] = hexString; | |
| } | |
| const char* const customPaletteNames[] = {"Random", "MoonModules", "Orange"}; | |
| for (int i = 0; i < sizeof(customPaletteNames) / sizeof(char*); i++) { | |
| JsonArray values = control["values"]; | |
| JsonObject object = values.add<JsonObject>(); | |
| object["name"] = customPaletteNames[i]; | |
| object["colors"] = ""; | |
| } | |
| // add palettes from palettes.h | |
| for (int i = 0; i < sizeof(palette_names) / sizeof(char*); i++) { | |
| JsonArray values = control["values"]; | |
| JsonObject object = values.add<JsonObject>(); | |
| object["name"] = palette_names[i]; | |
| String hexString = ""; | |
| const byte* palette = gGradientPalettes[i]; | |
| int j = 0; | |
| // Read 4-byte entries (index, r, g, b) until index == 255 | |
| while (j < 100) { // Safety limit | |
| for (int k = 0; k < 4; k++) { | |
| char buf[3]; | |
| sprintf(buf, "%02x", palette[j++]); | |
| hexString += buf; | |
| } | |
| // Check if we just wrote the final entry (index was 255) | |
| if (palette[j - 4] == 255) break; | |
| } | |
| object["colors"] = hexString; | |
| } | |
| control = addControl(controls, "palette", "palette"); // palette type | |
| control["default"] = 9; | |
| control["values"].to<JsonArray>(); | |
| const char* const builtInPaletteNames[] = {"Cloud", "Lava", "Ocean", "Forest", "Rainbow", "RainbowStripe", "Party", "Heat"}; | |
| const CRGBPalette16 builtInPalettes[] = {CloudColors_p, LavaColors_p, OceanColors_p, ForestColors_p, RainbowColors_p, RainbowStripeColors_p, PartyColors_p, HeatColors_p}; | |
| for (int i = 0; i < sizeof(builtInPaletteNames) / sizeof(char*); i++) { | |
| JsonArray values = control["values"]; | |
| JsonObject object = values.add<JsonObject>(); | |
| object["name"] = builtInPaletteNames[i]; | |
| // Convert CRGBPalette16 to hex string | |
| String hexString = ""; | |
| const CRGBPalette16 pal = builtInPalettes[i]; | |
| char buf[3]; | |
| for (int j = 0; j < 16; j++) { | |
| // Add index (0, 16, 32, ... 240) | |
| sprintf(buf, "%02x", j * 16); | |
| hexString += buf; | |
| // Add R, G, B | |
| sprintf(buf, "%02x", pal[j].r); | |
| hexString += buf; | |
| sprintf(buf, "%02x", pal[j].g); | |
| hexString += buf; | |
| sprintf(buf, "%02x", pal[j].b); | |
| hexString += buf; | |
| } | |
| // Add final entry at index 255 | |
| sprintf(buf, "%02x", 255); | |
| hexString += buf; | |
| sprintf(buf, "%02x", pal[15].r); | |
| hexString += buf; | |
| sprintf(buf, "%02x", pal[15].g); | |
| hexString += buf; | |
| sprintf(buf, "%02x", pal[15].b); | |
| hexString += buf; | |
| object["colors"] = hexString; | |
| } | |
| const char* const customPaletteNames[] = {"Random", "MoonModules", "Orange"}; | |
| for (int i = 0; i < sizeof(customPaletteNames) / sizeof(char*); i++) { | |
| JsonArray values = control["values"]; | |
| JsonObject object = values.add<JsonObject>(); | |
| object["name"] = customPaletteNames[i]; | |
| object["colors"] = ""; | |
| } | |
| // add palettes from palettes.h | |
| for (int i = 0; i < sizeof(palette_names) / sizeof(char*); i++) { | |
| JsonArray values = control["values"]; | |
| JsonObject object = values.add<JsonObject>(); | |
| object["name"] = palette_names[i]; | |
| String hexString = ""; | |
| const byte* palette = gGradientPalettes[i]; | |
| int j = 0; | |
| // Read 4-byte entries (index, r, g, b) until index == 255 | |
| const size_t kMaxPaletteBytes = 256; // allow larger gradients | |
| while (j + 3 < kMaxPaletteBytes) { | |
| for (int k = 0; k < 4; k++) { | |
| char buf[3]; | |
| sprintf(buf, "%02x", palette[j++]); | |
| hexString += buf; | |
| } | |
| // Check if we just wrote the final entry (index was 255) | |
| if (palette[j - 4] == 255) break; | |
| } | |
| object["colors"] = hexString; | |
| } |
🤖 Prompt for AI Agents
In `@src/MoonLight/Modules/ModuleLightsControl.h` around lines 281 - 355, The loop
that builds hexString for gGradientPalettes uses a hard cap while (j < 100)
which can truncate palettes—replace this fixed limit with a safe bounds check
tied to the actual palette byte length (or a defined MAX_PALETTE_BYTES) and
iterate in 4-byte steps until you encounter an index==255 termination;
specifically update the loop that reads from gGradientPalettes[i] (the variables
palette and j) so it ensures (j + 4) <= palette_length (or <= MAX_PALETTE_BYTES)
before reading the next 4 bytes, append the 4 bytes per entry, and break when
palette[j] (the entry index) == 255 to avoid truncation.
| byte tcp[76] = {255}; // WLEDMM: prevent out-of-range access in loadDynamicGradientPalette() | ||
| memcpy(tcp, (byte*)pgm_read_dword(&(gGradientPalettes[updatedItem.value.as<uint8_t>() - 11])), 72); | ||
| layerP.palette.loadDynamicGradientPalette(tcp); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
# Find files related to palette handling
fd -t f "\.h$|\.cpp$" | xargs rg -l "loadDynamicGradientPalette|gGradientPalettes" | head -20Repository: MoonModules/MoonLight
Length of output: 141
🏁 Script executed:
# Search for loadDynamicGradientPalette implementation details
rg -A 10 "loadDynamicGradientPalette" --type cpp --type hRepository: MoonModules/MoonLight
Length of output: 1361
🏁 Script executed:
# Check gradient palette definitions and sizes
rg -B 2 -A 5 "gGradientPalettes" --type cpp --type h | head -50Repository: MoonModules/MoonLight
Length of output: 2130
🏁 Script executed:
# Look for FastLED palette structure in the codebase
rg "CRGBPalette" --type cpp --type h | head -20Repository: MoonModules/MoonLight
Length of output: 546
🏁 Script executed:
# Read the palettes.h file to see palette definitions
cat src/MoonLight/Modules/palettes.h | head -100Repository: MoonModules/MoonLight
Length of output: 4786
🏁 Script executed:
# Search for the actual gradient palette definitions
rg -A 2 "gp =" src/MoonLight/Modules/palettes.h | head -40Repository: MoonModules/MoonLight
Length of output: 47
🏁 Script executed:
# Check what size gradients typically are by looking at their PROGMEM definitions
rg "PROGMEM|gp\[" src/MoonLight/Modules/palettes.h | head -20Repository: MoonModules/MoonLight
Length of output: 2895
🏁 Script executed:
# Count the actual byte size of all palette definitions
python3 << 'EOF'
import re
with open('src/MoonLight/Modules/palettes.h', 'r') as f:
content = f.read()
# Find all palette definitions
pattern = r'const byte (\w+)\[\] = \{([^}]+)\}'
matches = re.findall(pattern, content)
sizes = []
for name, data in matches:
# Count commas to get byte count
byte_count = len(data.split(','))
sizes.append((name, byte_count))
# Sort by size
sizes.sort(key=lambda x: x[1], reverse=True)
print("Top 15 largest palettes:")
for name, size in sizes[:15]:
print(f" {name}: {size} bytes")
print(f"\nTotal palettes: {len(sizes)}")
print(f"Max size: {max(s[1] for s in sizes)} bytes")
print(f"Min size: {min(s[1] for s in sizes)} bytes")
EOFRepository: MoonModules/MoonLight
Length of output: 151
🏁 Script executed:
# Check if there are any palettes >= 144 bytes
rg "const byte.*\[\] = \{" src/MoonLight/Modules/palettes.h | wc -lRepository: MoonModules/MoonLight
Length of output: 67
🏁 Script executed:
# Look for the loadDynamicGradientPalette implementation in FastLED
rg -B 5 -A 15 "loadDynamicGradientPalette" --type h | head -60Repository: MoonModules/MoonLight
Length of output: 2338
🏁 Script executed:
# Find the largest palette definitions
rg "const byte.*\[\] = \{" src/MoonLight/Modules/palettes.h -A 0 | while read line; do
name=$(echo "$line" | sed 's/.*const byte \([a-zA-Z_]*\).*/\1/')
# Extract just the declaration and count braces
size=$(rg "const byte $name\[\] = \{[^}]+\}" src/MoonLight/Modules/palettes.h | grep -o "{[^}]*}" | head -1 | tr ',' '\n' | wc -l)
echo "$size $name"
done | sort -rn | head -20Repository: MoonModules/MoonLight
Length of output: 592
🏁 Script executed:
# Search for documented sizes in the file
rg "Size: [0-9]+ bytes" src/MoonLight/Modules/palettes.h | sed 's/.*Size: \([0-9]*\).*/\1/' | sort -rn | uniqRepository: MoonModules/MoonLight
Length of output: 106
🏁 Script executed:
# Get specific info about the largest palettes mentioned
rg -B 3 "Size: [0-9]+ bytes" src/MoonLight/Modules/palettes.h | grep -E "(Size:|Gradient palette)" | paste - - | sort -t: -k2 -rn | head -20Repository: MoonModules/MoonLight
Length of output: 108
🏁 Script executed:
# Find which palette is 144 bytes
rg -B 5 "Size: 144 bytes" src/MoonLight/Modules/palettes.hRepository: MoonModules/MoonLight
Length of output: 351
🏁 Script executed:
# Get all palettes sorted by size to understand the distribution
rg -B 3 "Size: [0-9]+ bytes" src/MoonLight/Modules/palettes.h | rg "Size: [0-9]+" | sed 's/.*Size: \([0-9]*\).*/\1/' | sort -n | tail -20Repository: MoonModules/MoonLight
Length of output: 125
🏁 Script executed:
# Search for FastLED palette loading code in the repository
rg -i "loadDynamicGradientPalette|gradient.*palette" --type h --type cpp | grep -i "load\|terminator\|255" | head -20Repository: MoonModules/MoonLight
Length of output: 310
🏁 Script executed:
# Check the exact temperature_gp definition to understand its structure
rg -A 40 "Gradient palette.*temperature_gp" src/MoonLight/Modules/palettes.hRepository: MoonModules/MoonLight
Length of output: 1995
Copy gradient palettes completely until terminator, not with fixed byte limit.
The current code copies only 72 bytes, which truncates the temperature_gp palette (144 bytes) and removes its terminator (0xFF entry), causing loadDynamicGradientPalette() to read out-of-bounds. The buffer initialization byte tcp[76] = {255} only protects position 0; the rest is zero-initialized, not a valid terminator. Repack with a copy-until-terminator loop using pgm_read_byte() to safely handle palettes of any size up to the buffer limit.
🤖 Prompt for AI Agents
In `@src/MoonLight/Modules/ModuleLightsControl.h` around lines 425 - 427, The
fixed-size memcpy truncates palettes and drops the 0xFF terminator; change to
copying from PROGMEM byte-by-byte using pgm_read_byte() from
gGradientPalettes[index] (index computed as updatedItem.value.as<uint8_t>() -
11) into tcp until you encounter 0xFF or reach the tcp buffer limit, then ensure
tcp contains a valid 0xFF terminator before calling
layerP.palette.loadDynamicGradientPalette(tcp); also ensure tcp’s size is
sufficient for the largest expected palette and set the final byte to 0xFF if
the loop exits due to the size limit.
| // #include <Arduino.h> | ||
|
|
||
| const byte ib_jul01_gp[] = {0, 194, 1, 1, 94, 1, 29, 18, 132, 57, 131, 28, 255, 113, 1, 1}; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
cat -n src/MoonLight/Modules/palettes.h | head -40Repository: MoonModules/MoonLight
Length of output: 1977
🏁 Script executed:
rg "typedef.*byte|using.*byte|#define.*byte" --type cpp --type h -B2 -A2Repository: MoonModules/MoonLight
Length of output: 47
🏁 Script executed:
rg "^#include" src/MoonLight/Modules/palettes.hRepository: MoonModules/MoonLight
Length of output: 47
🏁 Script executed:
fd "Arduino.h" src/Repository: MoonModules/MoonLight
Length of output: 47
🏁 Script executed:
rg "Arduino\.h" src/ --type cpp --type hRepository: MoonModules/MoonLight
Length of output: 204
🏁 Script executed:
rg "\bbyte\b" src/MoonLight/Modules/palettes.hRepository: MoonModules/MoonLight
Length of output: 8405
🏁 Script executed:
git ls-files src/MoonLight/ | head -20Repository: MoonModules/MoonLight
Length of output: 866
🏁 Script executed:
cat -n src/MoonLight/Modules/palettes.h | head -50Repository: MoonModules/MoonLight
Length of output: 2535
🏁 Script executed:
rg "^\s*#include" src/MoonLight/Modules/palettes.h -A0Repository: MoonModules/MoonLight
Length of output: 47
🏁 Script executed:
rg "typedef.*uint8_t|using.*uint8_t" --type cpp --type hRepository: MoonModules/MoonLight
Length of output: 488
🏁 Script executed:
cat src/MoonBase/pal.h | head -50Repository: MoonModules/MoonLight
Length of output: 1474
🏁 Script executed:
wc -l src/MoonLight/Modules/palettes.hRepository: MoonModules/MoonLight
Length of output: 101
🏁 Script executed:
tail -20 src/MoonLight/Modules/palettes.hRepository: MoonModules/MoonLight
Length of output: 1759
🏁 Script executed:
grep -n "#include\|#define\|typedef\|using" src/MoonLight/Modules/palettes.h | head -20Repository: MoonModules/MoonLight
Length of output: 113
🏁 Script executed:
rg "type.*byte|byte.*type" src/ --type cpp --type hRepository: MoonModules/MoonLight
Length of output: 47
🏁 Script executed:
cat src/main.cpp | head -30Repository: MoonModules/MoonLight
Length of output: 1305
🏁 Script executed:
rg "palettes\.h" src/ --type cpp --type hRepository: MoonModules/MoonLight
Length of output: 212
🏁 Script executed:
grep -r "byte" src/MoonLight/Modules/palettes.h | head -5Repository: MoonModules/MoonLight
Length of output: 405
🏁 Script executed:
cat -n src/MoonLight/Modules/ModuleLightsControl.h | head -40Repository: MoonModules/MoonLight
Length of output: 1811
🏁 Script executed:
grep -n "#include" src/MoonLight/Modules/ModuleLightsControl.hRepository: MoonModules/MoonLight
Length of output: 284
Make byte definition self-contained.
This header relies on byte from Arduino, but Arduino.h isn't included. That makes compilation depend on include order and will fail outside Arduino builds. Add a guarded include or fallback typedef.
🛠️ Proposed fix
-// `#include` <Arduino.h>
+#if __has_include(<Arduino.h>)
+#include <Arduino.h>
+#else
+#include <cstdint>
+using byte = uint8_t;
+#endif📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // #include <Arduino.h> | |
| const byte ib_jul01_gp[] = {0, 194, 1, 1, 94, 1, 29, 18, 132, 57, 131, 28, 255, 113, 1, 1}; | |
| `#if` __has_include(<Arduino.h>) | |
| `#include` <Arduino.h> | |
| `#else` | |
| `#include` <cstdint> | |
| using byte = uint8_t; | |
| `#endif` | |
| const byte ib_jul01_gp[] = {0, 194, 1, 1, 94, 1, 29, 18, 132, 57, 131, 28, 255, 113, 1, 1}; |
🧰 Tools
🪛 Clang (14.0.6)
[error] 30-30: unknown type name 'byte'
(clang-diagnostic-error)
🤖 Prompt for AI Agents
In `@src/MoonLight/Modules/palettes.h` around lines 28 - 31, The header uses the
Arduino type `byte` for `ib_jul01_gp` but doesn't include Arduino.h, making
compilation order-dependent; update the header to be self-contained by adding a
guarded include or fallback typedef: check for a prior definition of `byte` (or
for ARDUINO macro) and if missing either include <Arduino.h> or typedef unsigned
char byte so `const byte ib_jul01_gp[]` has a defined type within this header.
| } else if (combined_code == codePaletteInc) { // palette increase | ||
| newState["palette"] = MIN(state.data["palette"].as<uint8_t>() + 1, 8 + 3 + 61); // 8 FastLED + 3 custom + 61 WLED palettes. to do: replace nr with max palette count | ||
| } else if (combined_code == codePaletteDec) { // palette decrease |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clamp max palette index to the last valid value.
Line 368 clamps to 72 (the palette count), but valid indices are 0–71. This allows palette=72, which will index past gGradientPalettes in ModuleLightsControl. Clamp to count − 1 (or a shared max constant).
🛠️ Proposed fix
- newState["palette"] = MIN(state.data["palette"].as<uint8_t>() + 1, 8 + 3 + 61); // 8 FastLED + 3 custom + 61 WLED palettes. to do: replace nr with max palette count
+ newState["palette"] = MIN(state.data["palette"].as<uint8_t>() + 1, (uint8_t)(8 + 3 + 61 - 1)); // max index = count-1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } else if (combined_code == codePaletteInc) { // palette increase | |
| newState["palette"] = MIN(state.data["palette"].as<uint8_t>() + 1, 8 + 3 + 61); // 8 FastLED + 3 custom + 61 WLED palettes. to do: replace nr with max palette count | |
| } else if (combined_code == codePaletteDec) { // palette decrease | |
| } else if (combined_code == codePaletteInc) { // palette increase | |
| newState["palette"] = MIN(state.data["palette"].as<uint8_t>() + 1, (uint8_t)(8 + 3 + 61 - 1)); // max index = count-1 | |
| } else if (combined_code == codePaletteDec) { // palette decrease |
🤖 Prompt for AI Agents
In `@src/MoonLight/Nodes/Drivers/D_Infrared.h` around lines 367 - 369, The clamp
for palette increase in the combined_code == codePaletteInc branch currently
uses MIN(state.data["palette"].as<uint8_t>() + 1, 8 + 3 + 61) which allows an
out‑of‑range index (72); change the ceiling to the last valid index by
subtracting 1 (i.e. use (8 + 3 + 61) - 1 or, better, a shared MAX_PALETTE_INDEX
constant) so newState["palette"] is clamped to 0..(count-1); update the same
logic location (the combined_code == codePaletteInc branch that sets
newState["palette"]) and ensure any other uses of the literal 8+3+61 are
replaced with the shared constant or consistently subtract 1.
backend ======= - virtual layer: add presetCorrection hook in forEachLightIndex (for lightPreset_RGB2040) - AudioSync: implement audio reactive palettes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/MoonLight/Layers/VirtualLayer.h (1)
134-138: Apply presetCorrection in the unmapped path for RGB2040 support.The
presetCorrectionapplies an index adjustment specific to RGB2040 preset (indexP += (indexP / 20) * 20), but this is only called in them_oneLightandm_moreLightscases. WhenoneToOneMappingis true (direct virtual-to-physical index mapping), the unmapped path is taken and callscallback(indexV)directly, bypassing the correction.Since RGB2040 preset is orthogonal to mapping type, it can be used with regular layouts that maintain 1:1 mapping. In that scenario, the preset correction would not be applied, resulting in incorrect index handling for RGB2040 LEDs. Add
presetCorrection(indexV)before the callback in the unmapped path.
🧹 Nitpick comments (2)
src/MoonLight/Nodes/Drivers/D_AudioSync.h (1)
69-91: Consider initializingvalueor adding a default case for defensive coding.If
palis outside the 0-2 range,valuerelies on CRGB's default constructor (which initializes to black). While currently safe since callers only pass 0-2, explicit initialization would make the intent clearer and guard against future changes.♻️ Suggested improvement
CRGB getCRGBForBand(int x, uint8_t* fftResult, int pal) { - CRGB value; + CRGB value = CRGB::Black; CHSV hsv; if (pal == 0) { // bit hacky to use palette id here, but don't want to litter the code with lots of different methods. TODO: add enum for palette creation typesrc/MoonLight/Layers/VirtualLayer.cpp (1)
84-87: Consider adding a brief comment explaining the RGB2040 physical layout.The formula
indexP += (indexP / 20) * 20creates a sparse physical mapping where lights 0-19 stay at 0-19, lights 20-39 map to 40-59, etc. A short comment explaining why this layout is needed for RGB2040 would aid maintainability.📝 Suggested documentation
void VirtualLayer::presetCorrection(nrOfLights_t& indexP) const { + // RGB2040 has physical layout with alternating 20-light segments: + // virtual [0..19] -> physical [0..19], virtual [20..39] -> physical [40..59], etc. if (layerP->lights.header.lightPreset == lightPreset_RGB2040) indexP += (indexP / 20) * 20; }
back end ======== - Module: fix bug in post/getUpdate ! Avoid missing updates - Effects: Implement ColorTwinkle, Plasma, Julia, Police
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/MoonLight/Nodes/Effects/E_WLED.h`:
- Around line 2002-2049: The loop() method uses the data pointer (accessing
data[index] and bitRead/bitWrite) without checking for null; add a guard at the
start of E_WLED::loop() that returns immediately if data == nullptr or layer ==
nullptr or layer->nrOfLights == 0 to match other effects (e.g.,
BouncingBallsEffect/TetrixEffect); this prevents dereferencing data before it is
allocated in onSizeChanged() — locate the data accesses in loop(), add the
null-check and early return, and keep existing behavior otherwise.
🧹 Nitpick comments (3)
src/MoonBase/Module.cpp (2)
289-293: Consider removing commented-out debug code.This commented-out logging block adds noise without providing value. If debugging is needed in the future, it can be easily re-added or enabled via a debug flag.
🧹 Suggested removal
bool changed = state.checkReOrderSwap("", state.data, newData, updatedItem); - // if (originId != "devicesserver" && originId != "tasksserver") { - // String ss; - // serializeJson(newData, ss); - // EXT_LOGD(ML_TAG, "newData %s from %s", ss.c_str(), originId.c_str()); - // } - // EXT_LOGD(ML_TAG, "update isNew %d changed %d", isNew, changed);
279-310: Early guard for empty data is a good defensive addition.The logic correctly short-circuits when
newDatais empty. One minor suggestion: inverting the condition would reduce nesting depth and improve readability.♻️ Optional: flatten structure with early return
StateUpdateResult ModuleState::update(JsonObject& newData, ModuleState& state, const String& originId) { updateOriginId = originId; - if (newData.size() != 0) { // in case of empty file - - // check which controls have updated - if (newData != state.data) { - UpdatedItem updatedItem; - - bool changed = state.checkReOrderSwap("", state.data, newData, updatedItem); - - if (state.compareRecursive("", state.data, newData, updatedItem)) { - if (changed) EXT_LOGW(ML_TAG, "checkReOrderSwap changed, compareRecursive also changed? %s", originId.c_str()); - changed = true; - } - - return (changed) ? StateUpdateResult::CHANGED : StateUpdateResult::UNCHANGED; - } else - return StateUpdateResult::UNCHANGED; - } else { + if (newData.size() == 0) { EXT_LOGW(MB_TAG, "empty newData %s", originId.c_str()); return StateUpdateResult::UNCHANGED; } + + // check which controls have updated + if (newData == state.data) { + return StateUpdateResult::UNCHANGED; + } + + UpdatedItem updatedItem; + bool changed = state.checkReOrderSwap("", state.data, newData, updatedItem); + + if (state.compareRecursive("", state.data, newData, updatedItem)) { + if (changed) EXT_LOGW(ML_TAG, "checkReOrderSwap changed, compareRecursive also changed? %s", originId.c_str()); + changed = true; + } + + return changed ? StateUpdateResult::CHANGED : StateUpdateResult::UNCHANGED; }src/MoonLight/Nodes/Effects/E_WLED.h (1)
2237-2253: Variable shadowing: localwidthshadows memberwidth.Line 2242 declares a local
uint16_t widththat shadows the member variableuint8_t width. While the code works (the member is used on the right side before shadowing takes effect), this is confusing and error-prone for future maintenance.♻️ Rename local variable for clarity
void police_base(uint32_t color1, uint32_t color2) { uint16_t delay = 1 + (1000 / 40 << 3) / layer->size.y; // longer segments should change faster uint32_t it = pal::millis() / map(speed, 0, 255, delay << 4, delay); uint16_t offset = it % layer->size.y; - uint16_t width = ((layer->size.y * (width + 1)) >> 9); // max width is half the strip - if (!width) width = 1; - for (int i = 0; i < width; i++) { + uint16_t pixelWidth = ((layer->size.y * (width + 1)) >> 9); // max width is half the strip + if (!pixelWidth) pixelWidth = 1; + for (int i = 0; i < pixelWidth; i++) { uint16_t indexR = (offset + i) % layer->size.y; uint16_t indexB = (offset + i + (layer->size.y >> 1)) % layer->size.y;
| void loop() override { | ||
| CRGB fastled_col, prev; | ||
| uint8_t brightness = 128; // strip.getBrightness() | ||
| fract8 fadeUpAmount = brightness > 28 ? 8 + (fadeSpeed >> 2) : 68 - brightness; | ||
| fract8 fadeDownAmount = brightness > 28 ? 8 + (fadeSpeed >> 3) : 68 - brightness; | ||
| for (uint16_t i = 0; i < layer->nrOfLights; i++) { | ||
| fastled_col = layer->getRGB(i); | ||
| prev = fastled_col; | ||
| uint16_t index = i >> 3; | ||
| uint8_t bitNum = i & 0x07; | ||
| bool fadeUp = bitRead(data[index], bitNum); | ||
|
|
||
| if (fadeUp) { | ||
| CRGB incrementalColor = fastled_col; | ||
| incrementalColor.nscale8_video(fadeUpAmount); | ||
| fastled_col += incrementalColor; | ||
|
|
||
| if (fastled_col.red == 255 || fastled_col.green == 255 || fastled_col.blue == 255) { | ||
| bitWrite(data[index], bitNum, false); | ||
| } | ||
| layer->setRGB(i, fastled_col); | ||
|
|
||
| if (layer->getRGB(i) == prev) { // fix "stuck" pixels | ||
| fastled_col += fastled_col; | ||
| layer->setRGB(i, fastled_col); | ||
| } | ||
| } else { | ||
| fastled_col.nscale8(255 - fadeDownAmount); | ||
| layer->setRGB(i, fastled_col); | ||
| } | ||
| } | ||
|
|
||
| for (uint16_t j = 0; j <= layer->nrOfLights / 50; j++) { | ||
| if (random8() <= spawnSpeed) { | ||
| for (uint8_t times = 0; times < 5; times++) { // attempt to spawn a new pixel 5 times | ||
| int i = random16(layer->nrOfLights); | ||
| if (layer->getRGB(i) == 0) { | ||
| fastled_col = ColorFromPalette(layerP.palette, random8(), 64, NOBLEND); | ||
| uint16_t index = i >> 3; | ||
| uint8_t bitNum = i & 0x07; | ||
| bitWrite(data[index], bitNum, true); | ||
| layer->setRGB(i, fastled_col); | ||
| break; // only spawn 1 new pixel per frame per 50 LEDs | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing null check for data pointer before use.
The loop() method accesses data[index] without first checking if data is nullptr. If allocation fails in onSizeChanged() or if loop() is called before onSizeChanged(), this will cause a crash. Other effects in this file guard against this (e.g., BouncingBallsEffect at line 38, TetrixEffect at line 991).
🐛 Proposed fix
void loop() override {
+ if (!data) return;
+
CRGB fastled_col, prev;
uint8_t brightness = 128; // strip.getBrightness()🤖 Prompt for AI Agents
In `@src/MoonLight/Nodes/Effects/E_WLED.h` around lines 2002 - 2049, The loop()
method uses the data pointer (accessing data[index] and bitRead/bitWrite)
without checking for null; add a guard at the start of E_WLED::loop() that
returns immediately if data == nullptr or layer == nullptr or layer->nrOfLights
== 0 to match other effects (e.g., BouncingBallsEffect/TetrixEffect); this
prevents dereferencing data before it is allocated in onSizeChanged() — locate
the data accesses in loop(), add the null-check and early return, and keep
existing behavior otherwise.
|
@coderabbitai, can you review the update on postUpdate / getUpdate in latest commit, can you confirm it was faulty before and okay now? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 33 🏁 Script executed: Length of output: 239 🏁 Script executed: Length of output: 3256
The Problem in the Old CodeThe old // OLD CODE - BUGGY
if (xSemaphoreTake(updateMutex, portMAX_DELAY) == pdTRUE) {
this->updatedItem = updatedItem; // ⚠️ Overwrites without checking updatePending!
updatePending = true;
xSemaphoreGive(updateMutex);
}The bug: If a producer called How the New Code Fixes ItThe new implementation properly serializes updates: // NEW CODE - CORRECT
while (true) {
if (xSemaphoreTake(updateMutex, pdMS_TO_TICKS(10)) == pdTRUE) {
if (!updatePending) { // ✓ Only write when previous update consumed
this->updatedItem = updatedItem;
updatePending = true;
xSemaphoreGive(updateMutex);
break;
}
xSemaphoreGive(updateMutex);
}
vTaskDelay(pdMS_TO_TICKS(1)); // Wait and retry
}Key improvements:
The fix is correct and addresses a real concurrency issue. Well done on catching and fixing this! 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/MoonLight/Nodes/Effects/E_WLED.h`:
- Around line 1999-2001: Add a destructor to class E_WLED that frees the heap
buffer allocated in onSizeChanged: if the member uint8_t* data is non-null, call
delete[] data, set data to nullptr and dataSize to 0; also consider marking copy
constructor/assignment as deleted or implement proper copy/move semantics to
avoid double-free/ownership bugs since data is a raw pointer.
♻️ Duplicate comments (1)
src/MoonLight/Modules/palettes.h (1)
25-31: Makebytedefinition self-contained.This header uses
byte(an Arduino-specific type alias foruint8_t) without includingArduino.h. This causes compilation failures in non-Arduino build environments, as confirmed by static analysis.🛠️ Proposed fix
`#ifndef` Palettes_h `#define` Palettes_h -// `#include` <Arduino.h> +#ifndef byte + `#include` <cstdint> + using byte = uint8_t; +#endif const byte ib_jul01_gp[] = {0, 194, 1, 1, 94, 1, 29, 18, 132, 57, 131, 28, 255, 113, 1, 1};
| uint8_t* data = nullptr; | ||
| size_t dataSize = 0; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing destructor causes memory leak.
The data pointer allocated in onSizeChanged is never freed. Add a destructor to prevent memory leaks when the effect is destroyed.
🐛 Proposed fix
uint8_t* data = nullptr;
size_t dataSize = 0;
+ ~ColorTwinkleEffect() override {
+ if (data) freeMB(data, "data");
+ }
+
void onSizeChanged(const Coord3D& prevSize) override { reallocMB2<uint8_t>(data, dataSize, (layer->nrOfLights + 7) >> 3, "data"); }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uint8_t* data = nullptr; | |
| size_t dataSize = 0; | |
| uint8_t* data = nullptr; | |
| size_t dataSize = 0; | |
| ~ColorTwinkleEffect() override { | |
| if (data) freeMB(data, "data"); | |
| } | |
| void onSizeChanged(const Coord3D& prevSize) override { reallocMB2<uint8_t>(data, dataSize, (layer->nrOfLights + 7) >> 3, "data"); } |
🤖 Prompt for AI Agents
In `@src/MoonLight/Nodes/Effects/E_WLED.h` around lines 1999 - 2001, Add a
destructor to class E_WLED that frees the heap buffer allocated in
onSizeChanged: if the member uint8_t* data is non-null, call delete[] data, set
data to nullptr and dataSize to 0; also consider marking copy
constructor/assignment as deleted or implement proper copy/move semantics to
avoid double-free/ownership bugs since data is a raw pointer.
|
@coderabbitai, can you review the blur functions in virtual layer.h as they do not work as intended, eg blackhole calls blur2d but is not showing blur |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 3731 🏁 Script executed: Length of output: 2258 🏁 Script executed: Length of output: 667 🏁 Script executed: Length of output: 7224 🏁 Script executed: Length of output: 6475 🏁 Script executed: Length of output: 4957 🏁 Script executed: Length of output: 2985 🏁 Script executed: Length of output: 3328 🏁 Script executed: Length of output: 882 🏁 Script executed: Length of output: 1076 🏁 Script executed: Length of output: 104 🏁 Script executed: Length of output: 1762
The ProblemLooking at lines 320-361 in VirtualLayer.h, both blur functions follow this pattern: for (uint16_t i = 0; i < width; i++) { // (or height for columns)
CRGB cur = getRGB(...);
CRGB part = cur;
part.nscale8(seep);
cur.nscale8(keep);
cur += carryover;
if (i) addRGB(..., part); // add to PREVIOUS pixel
setRGB(..., cur);
carryover = part; // save for next iteration
}
// ⚠️ Loop ends here - carryover is lost!After the loop completes,
The FixAdd the final carryover write after each inner loop: For blurRows (after line 341, before the closing for (uint16_t row = 0; row < height; row++) {
CRGB carryover = CRGB::Black;
for (uint16_t i = 0; i < width; i++) {
// ... existing code ...
carryover = part;
}
// Add this line:
addRGB(Coord3D(width - 1, row), carryover);
}For blurColumns (after line 360, before the closing for (uint16_t col = 0; col < width; ++col) {
CRGB carryover = CRGB::Black;
for (uint16_t i = 0; i < height; ++i) {
// ... existing code ...
carryover = part;
}
// Add this line:
addRGB(Coord3D(col, height - 1), carryover);
}This ensures the blur properly distributes across the entire matrix and should make the blur visible in Blackhole and other effects. Would you like me to create a code change plan to fix both functions? 🧠 Learnings used |
|
@coderabbitai, that is better indeed , is it also possible to keep the original pixels as bright as they were and also the blur |
|
Tip For best results, initiate chat on the files or code changes.
For additive blur (bright originals + blur on top), we need to change the algorithm: Current Algorithm (Energy-Redistributing)keep = 255 - blur_amount; // 83 (~33%)
seep = blur_amount >> 1; // 86 (~34%)
cur.nscale8(keep); // ⚠️ Dims to 33%
cur += carryover;New Algorithm (Additive Blur)Remove the blurRows (around line 327-341): void blurRows(uint8_t blur_amount) {
uint8_t seep = blur_amount >> 1; // Neighbor contribution
// NO keep scaling - pixels stay 100% bright
for (uint16_t row = 0; row < height; row++) {
CRGB carryover = CRGB::Black;
for (uint16_t i = 0; i < width; i++) {
CRGB cur = getRGB(Coord3D(i, row));
CRGB part = cur;
part.nscale8(seep);
// DON'T scale cur - keep original brightness
cur += carryover;
if (i) addRGB(Coord3D(i - 1, row), part);
setRGB(Coord3D(i, row), cur);
carryover = part;
}
addRGB(Coord3D(width - 1, row), carryover); // Final carryover
}
}blurColumns (similar change around line 346-360): void blurColumns(uint8_t blur_amount) {
uint8_t seep = blur_amount >> 1;
for (uint16_t col = 0; col < width; ++col) {
CRGB carryover = CRGB::Black;
for (uint16_t i = 0; i < height; ++i) {
CRGB cur = getRGB(Coord3D(col, i));
CRGB part = cur;
part.nscale8(seep);
// DON'T scale cur
cur += carryover;
if (i) addRGB(Coord3D(col, i - 1), part);
setRGB(Coord3D(col, i), cur);
carryover = part;
}
addRGB(Coord3D(col, height - 1), carryover); // Final carryover
}
}This way:
The trade-off: brightness increases, so you may want to reduce 🧠 Learnings used💡 Use |
|
@coderabbitai, if we don't scale cur , the whole screen fills up with solid color,if we scale cur, to much blur means nothing is shown. Is there a better algorithm which keeps the brightness not not fill up the whole screen ? |
|
Tip For best results, initiate chat on the files or code changes.
The Solution: Weighted Average Box BlurInstead of scaling then adding, compute a weighted average of each pixel with its neighbors: void blurRows(uint8_t blur_amount) {
if (blur_amount == 0) return;
// Convert blur_amount to a fraction (0-255 → 0.0-1.0)
uint8_t weight = blur_amount;
for (uint16_t row = 0; row < height; row++) {
CRGB prev = CRGB::Black;
CRGB cur = getRGB(Coord3D(0, row));
for (uint16_t i = 0; i < width; i++) {
CRGB next = (i < width - 1) ? getRGB(Coord3D(i + 1, row)) : CRGB::Black;
// Weighted average: center pixel + neighbors
CRGB result = cur; // Start with 100% current pixel
// Add weighted contribution from neighbors
CRGB neighborSum = prev;
neighborSum += next;
neighborSum.nscale8_video(weight); // Scale by blur_amount
// Blend: keep most of original, add scaled neighbors
result.nscale8_video(255 - weight);
result += neighborSum.nscale8_video(weight >> 1); // Divide by 2 (two neighbors)
setRGB(Coord3D(i, row), result);
prev = cur;
cur = next;
}
}
}Wait, that's still complex. Let me give you a simpler, energy-conserving approach that FastLED uses: Simpler Solution: Three-Tap Box Blurvoid blurRows(uint8_t blur_amount) {
if (blur_amount == 0) return;
uint8_t keep = 255 - blur_amount;
uint8_t seep = blur_amount >> 1; // Divide by 2 for two neighbors
for (uint16_t row = 0; row < height; row++) {
CRGB carryover = CRGB::Black;
for (uint16_t i = 0; i < width; i++) {
CRGB cur = getRGB(Coord3D(i, row));
// Scale down current pixel
CRGB part = cur;
part.nscale8(seep);
cur.nscale8(keep);
// Add carryover from previous pixel
cur += carryover;
// Write result
setRGB(Coord3D(i, row), cur);
// Save for next iteration
carryover = part;
}
// Wrap final carryover to first pixel to conserve energy
if (width > 1) {
addRGB(Coord3D(0, row), carryover);
}
}
}Key changes:
Apply the same pattern to This is energy-conserving: no brightness is lost or gained, it's just redistributed smoothly. The wrapping prevents the "disappearing" effect while the scaling prevents the "filling" effect. Would you like me to create a code change plan for both 🧠 Learnings used |
|
@coderabbitai, for some reason class OctopusEffect is showing random pixels while it worked fine earlier, can you see what might have changed which caused this? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1032 🏁 Script executed: Length of output: 3938 🏁 Script executed: Length of output: 5135
The ProblemLooking at the code flow:
Result: Random pixel colors from uninitialized memory. The FixCall void onSizeChanged(const Coord3D& prevSize) override {
reallocMB2<Map_t>(rMap, rMapSize, layer->size.x * layer->size.y, "rMap");
if (rMap) setRMap(); // Initialize after allocation
}This ensures rMap is always properly initialized before Would you like me to create a code change plan to fix this? 🧠 Learnings used |
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.